-
Notifications
You must be signed in to change notification settings - Fork 185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
formatNumber; filter log tickFormat function #2078
Conversation
507264a
to
d9a98e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the feature — I hope we'll find a way to solve #384 too!
Two small comments/questions.
@@ -652,7 +652,7 @@ function inferTextChannel(scale, data, ticks, tickFormat, anchor) { | |||
// possible, or the default ISO format (2014-01-26). TODO We need a better way | |||
// to infer whether the ordinal scale is UTC or local time. | |||
export function inferTickFormat(scale, data, ticks, tickFormat, anchor) { | |||
return typeof tickFormat === "function" | |||
return typeof tickFormat === "function" && !(scale.type === "log" && scale.tickFormat) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return typeof tickFormat === "function" && !(scale.type === "log" && scale.tickFormat) | |
return typeof tickFormat === "function" && scale.type !== "log" |
Nit: I'm not sure about the case for the second part of the test. When scale.type is "log", the scale is an instance of d3.scaleLog() and it has a tickFormat method, which the user can't nullify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn’t do this because I didn’t want to assume that log scales necessarily implement scale.tickFormat
. That is currently true, but if this assumption doesn’t hold in the future, this would break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error may be mine, but this "invalid" message comes from the format in many European languages returning 2 000
which is not accepted in SVG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem comes from running beautify.html
on the generated SVG, meaning that the SVG is assumed to be in HTML. We’ll have to find a way to disable that for tests.
Edit: Actually it’s from JSDOM’s outerHTML
serialization.
Fixes #2077. Also exposes Plot.formatNumber(locale).